HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode#2470
HADOOP-16524. Reloading SSL keystore for both DataNode and NameNode#2470saintstack merged 3 commits intoapache:trunkfrom
Conversation
|
🎊 +1 overall
This message was automatically generated. |
| conf.getLong(FileBasedKeyStoresFactory.SSL_STORES_RELOAD_INTERVAL_TPL_KEY, | ||
| FileBasedKeyStoresFactory.DEFAULT_SSL_STORES_RELOAD_INTERVAL); | ||
|
|
||
| this.configurationChangeMonitor = storesReloadInterval > 0 |
There was a problem hiding this comment.
nit: this.configurationChangeMonitor is already Optional.empty? So, change this to be if (storesReloadInterval > 0) {....}?
| } | ||
|
|
||
| private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval, | ||
| SslContextFactory.Server sslContextFactory) { |
There was a problem hiding this comment.
Keep the style of the backing class... It doesn't do these big right-shifts on parameters that go to the second line. Be careful w/ line lengths too. The backing file seems to do 80 chars.
| try { | ||
| sslContextFactory.reload(factory -> { }); | ||
| } | ||
| catch (Exception ex) { |
There was a problem hiding this comment.
nit: See the formatting for catch in the rest of the file. It does not do this.
|
|
||
| private java.util.Timer makeConfigurationChangeMonitor(long reloadInterval, | ||
| SslContextFactory.Server sslContextFactory) { | ||
| java.util.Timer timer = new java.util.Timer("SSL Certificates Store Monitor", true); |
There was a problem hiding this comment.
Why the full qualification of Timer here and as function return? You have imported java.util.Timer so no need of these 'java.util' prefixes?
| * if the trust certificates keystore file changes, the {@link TrustManager} | ||
| * is refreshed with the new trust certificate entries (using a | ||
| * {@link ReloadingX509TrustManager} trustmanager). | ||
| * If either the truststore or the keystore certificates file changes, it would be refreshed |
...ct/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
Show resolved
Hide resolved
| private void createTrustManagersFromConfiguration(SSLFactory.Mode mode, | ||
| String truststoreType, | ||
| String truststoreLocation, | ||
| long storesReloadInterval) |
There was a problem hiding this comment.
Does the rest of this class do this extreme rightward shifting of parameters?
There was a problem hiding this comment.
Hmm... maybe it does. Ignore the above comment then.
There was a problem hiding this comment.
Yeah, but it's true the most of the code base doesn't have it. Should I change? I don't mind...just inadvertently sneaking in my old habits I suppose....
There was a problem hiding this comment.
Seems like your code habits align w/ the backing file here. I'd leave it as is.
saintstack
left a comment
There was a problem hiding this comment.
Looking good. A few nits in below.
...ct/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
Show resolved
Hide resolved
| storesReloadInterval); | ||
|
|
||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug(mode.toString() + " TrustStore: " + truststoreLocation); |
There was a problem hiding this comment.
Log the interval found in config here too?
...ct/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileBasedKeyStoresFactory.java
Show resolved
Hide resolved
| private void createTrustManagersFromConfiguration(SSLFactory.Mode mode, | ||
| String truststoreType, | ||
| String truststoreLocation, | ||
| long storesReloadInterval) |
There was a problem hiding this comment.
Hmm... maybe it does. Ignore the above comment then.
| import java.util.function.Consumer; | ||
|
|
||
| /** | ||
| * <p> |
There was a problem hiding this comment.
No need of the
wrappers.
...ject/hadoop-common/src/main/java/org/apache/hadoop/security/ssl/FileMonitoringTimerTask.java
Show resolved
Hide resolved
...hadoop-common/src/main/java/org/apache/hadoop/security/ssl/ReloadingX509KeystoreManager.java
Show resolved
Hide resolved
|
|
||
| private String type; | ||
| private String storePassword; | ||
| private String keyPassword; |
| } | ||
|
|
||
| @Override | ||
| public String chooseEngineClientAlias(String[] strings, Principal[] principals, SSLEngine sslEngine) { |
| this.keyManagerRef.set(loadKeyManager(path)); | ||
| } catch (Exception ex) { | ||
| // The Consumer.accept interface forces us to convert to unchecked | ||
| throw new RuntimeException(ex); |
There was a problem hiding this comment.
What will happen when this comes out? Where will it be caught? Will it cause damage causing process exit or thread exit?
There was a problem hiding this comment.
It gets caught by the FileMonitoringTimerTask and from there it's logged. It won't cause any damage, but it will keep logging the error on every reload interval if it's not resolved, potentially filling the logs and disk space.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
left a comment
There was a problem hiding this comment.
LGTM.
Have you tried it? If so, want to dump in here what you've done?
The original patch posted did not cover DN (See JIRA). This works for DN and NN?
Thanks.
| private void createTrustManagersFromConfiguration(SSLFactory.Mode mode, | ||
| String truststoreType, | ||
| String truststoreLocation, | ||
| long storesReloadInterval) |
There was a problem hiding this comment.
Seems like your code habits align w/ the backing file here. I'd leave it as is.
Yes, both DN and NN are covered and this was integration tested (manually) in a pre-prod environment on all nodes. DevOps team happy with it. Thanks so much for the review! |
|
Rerunning tests to see if failures related (Getting ready to merge) |
|
🎊 +1 overall
This message was automatically generated. |
…pache#2470) Co-authored-by: Borislav Iordanov <[email protected]> Signed-off-by: stack <[email protected]>
…2470) (#2609) Co-authored-by: Borislav Iordanov <[email protected]> Signed-off-by: stack <[email protected]> Co-authored-by: Borislav Iordanov <[email protected]> Co-authored-by: Borislav Iordanov <[email protected]>
|
@saintstack Thanks for your contribution. |
Addresses https://issues.apache.org/jira/browse/HADOOP-16524, in addition covering the DataNode use case.
If this PR is accepted, I would need guidance where/how to update the docs with that new configuration parameter.